Skip to content

Fix application profile link mess #410

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Jan 18, 2025
Merged

Fix application profile link mess #410

merged 8 commits into from
Jan 18, 2025

Conversation

@ccli8
Copy link

@ccli8 ccli8 commented Dec 30, 2024 •
edited
Loading

Summary of changes

This PR tries to address #407, including:

  1. Provide one approach to address cmake link mess with mbed-core-flags/
    mbed-rtos-flags whose links or not depends on selected application profile,
    full or bare-metal.
    1. Add target config option application-profile for application to
      select which application profile to use. Applications are responsible
      for make this target config option consitent with link selection of
      mbed-os/mbed-baremetal.
    2. Add cmake interface library mbed-application-profile-flags. It is to
      substitute for mbed-core-flags/mbed-rtos-flags. Library camke targets
      should change to link this one to build upon correct application profile
      selection.
  2. All in-tree library targets link mbed-application-profile-flags instead of mbed-core-flags/mbed-rtos-flags to build upon correct application profile.

Impact of changes

To link mbed-baremetal, target.application-profile must be set to bare-metal in mbed_app.json5.

Pull request type 

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

@ccli8 ccli8 force-pushed the build_fix_app_profile_link_mess branch from 1f33f26 to 219c536 Compare December 30, 2024 10:09
@ccli8 ccli8 force-pushed the build_fix_app_profile_link_mess branch 2 times, most recently from 5feb822 to 1c8f00e Compare December 31, 2024 06:42
@multiplemonomials
Copy link
Collaborator

multiplemonomials commented Jan 1, 2025 •
edited
Loading

Woah, this is a lot to take on! Thank you for working on this!

Haven't looked at the code yet but just going on the description:

  • Could we rename mbed-application-profile-flags to either mbed-os-flags or mbed-flags? Going to be typing that name 100000 times so I'd like it a bit shorter, and that doesn't lose information.
  • We will need to provide an alias so that libraries and applications that use mbed-core-flags will still work. Luckily CMake makes that easy to do with add_library(ALIAS)

@ccli8 ccli8 force-pushed the build_fix_app_profile_link_mess branch from 1c8f00e to 3301815 Compare January 2, 2025 01:30
@ccli8
Copy link
Author

ccli8 commented Jan 2, 2025 •
edited
Loading

Could we rename mbed-application-profile-flags to either mbed-os-flags or mbed-flags? Going to be typing that name 100000 times so I'd like it a bit shorter, and that doesn't lose information.

That's fine. Rename mbed-application-profile-flags to mbed-os-flags.

We will need to provide an alias so that libraries and applications that use mbed-core-flags will still work. Luckily CMake makes that easy to do with add_library(ALIAS)

There is compatible concern. My original idea is:

  • new mbed-core-flags = old mbed-core-flags
  • new mbed-rtos-flags = old mbed-rtos-flags
  • added mbed-os-flags = old mbed-core-flags + old mbed-rtos-flags if target.application-profile isfull.

To achieve above compatible, updated idea would become:

  • new mbed-core-flags = old mbed-core-flags + old mbed-rtos-flags if target.application-profile isfull.
  • new mbed-rtos-flags = old mbed-rtos-flags
  • added mbed-os-flags = new mbed-core-flags or become unnecessary

That is, mbed-core-flags is overloaded dependent on target.application-profile. And only one of mbed-os and mbed-baremetal target would be created dependent on target.application-profile, but not both. For example, when target.application-profile is full, mbed-baremetal cannot link mbed-core-flags because mbed-core-flags also links mbed-rtos-flags.
.

@multiplemonomials
Copy link
Collaborator

multiplemonomials commented Jan 2, 2025 •
edited
Loading

Makes sense to me! I definitely think that, depending on the application profile setting, we should make only the mbed-baremetal or mbed-os cmake targets available.

@ccli8 ccli8 force-pushed the build_fix_app_profile_link_mess branch from 3301815 to b8fa716 Compare January 3, 2025 03:14
@ccli8
Copy link
Author

ccli8 commented Jan 3, 2025

Update as below:

  1. Drop mbed-application-profile-flags (or its renamed versions mbed-os-flags). It is replaced with overloaded mbed-core-flags.
  2. Overload mbed-core-flags/mbed-core-sources so that they will involve mbed-rtos-flags/mbed-rtos-sources or not, depending on target.application-profile setting.
  3. All libraries must link mbed-core-flags and no mbed-rtos-flags, or the link mess issue remains.
  4. Both mbed-os and mbed-baremetal are still created, but one will become invalid and get caught on application link, depending on target.application-profile setting.

multiplemonomials
multiplemonomials reviewed Jan 4, 2025
View reviewed changes
@JohnK1987
Copy link
Member

JohnK1987 commented Jan 5, 2025

In targets.json is also this parameter

mbed-os/targets/targets.json5

Lines 32 to 33 in 722c2f1

"supported_application_profiles": [
"full"

I'm not sure if this has an use case at this moment.

@ccli8 ccli8 force-pushed the build_fix_app_profile_link_mess branch from 56d3a96 to f164549 Compare January 6, 2025 02:19
@ccli8
Copy link
Author

ccli8 commented Jan 6, 2025

In targets.json is also this parameter supported_application_profiles

The added application-profile config option and its supported options full and bare-metal just follows the naming of supported_application_profiles.

@ccli8 ccli8 force-pushed the build_fix_app_profile_link_mess branch from a658bfc to 08624c0 Compare January 6, 2025 09:38
multiplemonomials
multiplemonomials reviewed Jan 9, 2025
View reviewed changes
multiplemonomials
multiplemonomials reviewed Jan 9, 2025
View reviewed changes
multiplemonomials
multiplemonomials reviewed Jan 9, 2025
View reviewed changes
multiplemonomials
multiplemonomials reviewed Jan 9, 2025
View reviewed changes
ccli8 added 6 commits January 10, 2025 09:41
@ccli8
Support application profile link selection
8cd8e7a 
This provides one approach to address cmake link mess with
mbed-core-flags/mbed-rtos-flags whose links or not depends on selected
application profile, full or bare-metal.
1.  Add target config option application-profile for application to
    select which application profile to use. Applications are responsible
    for making this target config option consitent with link selection of
    mbed-os/mbed-baremetal.
2.  Overload mbed-core-flags/mbed-core-sources so that they can involve
    mbed-rtos-flags/mbed-rtos-sources or not depending on
    target.application-profile setting
@ccli8
Guard from mismatch of target.application-profile and mbed-os/mbed-ba…
fe241ea 
…remetal

This guards from mismatch of target.application-profile and link
libraries mbed-os/mbed-baremetal.
@ccli8
Make all libraries link mbed-core-flags instead of mbed-rtos-flags
425f0bb 
With mbed-core-flags overloaded to match target.application-profile setting,
all libraries should link mbed-core-flags instead of mbed-rtos-flags so that
they build upon correct application profile, involving mbed-rtos-flags or not.
@ccli8
Make mbed-os link mbed-core-flags/mbed-core-sources only
62a2efc 
With mbed-core-flags/mbed-core-sources being overloaded to match
target.application-profile setting, mbed-os should link only
mbed-core-flags/mbed-core-sources which will involve mbed-rtos-flags/
mbed-rtos-sources as config.
@ccli8
Remove MBED_GREENTEA_TEST_BAREMETAL from target.application-profile
d06791c 
MBED_GREENTEA_TEST_BAREMETAL can become deprecated and be removed. The
'auto' option of target.application-profile needn't respect it.
@ccli8
Respect target.application-profile setting for Greentea build
c35976f 
MBED_GREENTEA_TEST_BAREMETAL can become deprecated and be removed.
Respect target.application-profile setting instead to resolve link
library.
@ccli8 ccli8 force-pushed the build_fix_app_profile_link_mess branch from 08624c0 to c35976f Compare January 10, 2025 02:29
multiplemonomials
multiplemonomials approved these changes Jan 18, 2025
View reviewed changes
Copy link
Collaborator

@multiplemonomials multiplemonomials left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making this fix!

@multiplemonomials multiplemonomials merged commit a468f27 into mbed-ce:master Jan 18, 2025
52 checks passed
@multiplemonomials multiplemonomials mentioned this pull request Jan 18, 2025
Open
@ccli8 ccli8 deleted the build_fix_app_profile_link_mess branch January 20, 2025 05:40
@multiplemonomials multiplemonomials mentioned this pull request Mar 28, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@multiplemonomials multiplemonomials multiplemonomials approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ccli8 @multiplemonomials @JohnK1987